🚫 [reviewdog] <com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck> _{reported by reviewdog 🐶}
'{' at column 21 should have line break after.

Don't tell me what to do, reviewdog. That was formatted like that on purpose, to emphasize the fact that we don't care about that particular exception at that stage of the test.

As per your comment, IMO this method should be tagged @deprecated to indicate its imminent removal.

The integration test should be in the same package as the class (it already is in ...export, so it should move to ...export.ddi, the package this class is a part of). Then the method can be made package private and is accessible to the test right away without exposing it.

Is this a comment to explain the iterator pattern? If so, please use human language and remove the code block ;-)

This method is missing Javadocs explaining scope, reasoning and behavior.

Also:

1. I encourage you to use an empty list instead of a null value, which IMO is much more Javaish.
2. There is no null check on the dataset parameter.
3. Why not use DatasetVersion and split the logic in two methods? The getExport() method uses a DatasetVersion, thus IMO using a DatasetVersion + list of formats is more aligned. There can be a second method with Dataset + list of formats for convenience, that handles the business logic of which version may be exported (calling the one with version + formats internally).

For Johannes I will be adding a method with cardinality in the other direction: one format, multiple versions. Eventually, we will refactor the service to support exporting arbitrary versions, where it also makes sense to have version + formats.

No. The DatasetExportQuery has predicates only for the dataset's own metadata:

Predicates for controlling which dataset metadata is included in an export and at what level of detail.

The dataset export query may have a distinct FileExportQuery containing detailed information as its own predicates about the file metadata. At the moment, there is only one case where this method may return false:

1. There is a file export query within the dataset export query and it has a predicate "SKIP_FILES". (Otherwise it's either all files, public and/or tabular only).

We might need to tighten the definition in the export query spec, here's what it says now:

File metadata shaping is optional: if no FileExportQuery is provided, methods that include file metadata will apply their own defaults. Methods that do not return file metadata will ignore any nested FileExportQuery.

At time of writing, this seemed fine. But maybe it isn't? Should we do an explicit default? Do I remember correctly that in the past getDatasetJson() did not return file level metadata? (Then we should keep this default for backward compatibility, changing it would require a breaking change - and we shouldn't do those at this point.)

My gut tells me: if we really want to cache here, let's use a thread-safe map and the query as the key. In the edge case that someone really wants to retrieve the metadata again, they get a consistent result, as the query might be different the next time around!

The behavior here is not aligned with exportFormats. With that method using a null list will clear all, here you don't even check for null, risking an NPE.

I'd argue the same way as exportFormats: use a List, either empty or filled, disallow null. If empty, clear all.

This is a redundant method. People can just call the other defined methods. If you really want this for convenience, don't add a second implementation.

Please do not add this test here. It's an integration test, not an API test. For historic reasons, this file has been around and collects these API tests. It should be torched with fire.

Your performance test will be automatically picked up for execution during mvn verify -Dit.groups=performance

If you feel we should make performance tests a part of the regular Github Action flow, let's add it there! (currently only integration and migration are picked up, see pom.xml property it.groups)

As user input, the list of formats shall be verified on this code level, before entering any services.

Side note: The try/catch thing around everything is an anti-pattern and always has been. Let's not fix it here, but I wanted to repeat this, as it is an anti pattern all over our API code.